Develop by mForkosh · Pull Request #322 · mate-academy/node_move-files

mForkosh · 2025-09-06T17:31:58Z

No description provided.

mateacademy-ai-mentor

Review Summary

Decision: REQUEST_CHANGES — Several critical, blocking requirements are missing in src/app.js, which can lead to incorrect behavior and failing tests.

Critical issues to fix (with suggestions):

Argument validation (exactly two args)
- Where: src/app.js, near the CLI arg parsing (top of file, around where process.argv.slice(2) is used).
- Why: Requirement says the app must verify exactly two positional arguments (source, destination) and throw if not.
- How: Check args.length !== 2 and throw an Error like new Error('Usage: node index <source> <destination>'). Ensure process exits non‑zero.
Source exists and is a file (not a directory)
- Where: Before attempting fs.renameSync (same file, after parsing args).
- Why: Must throw if source does not exist or is not a file; directories are not supported.
- How: Use fs.existsSync(source) and fs.statSync(source).isFile(). If false, throw: new Error('Source must exist and be a file').
Disallow moving directories (explicit check)
- Where: Together with source validation.
- Why: Requirement: only files supported; directories must error.
- How: If fs.statSync(source).isDirectory(), throw an error.
Destination ending with slash must refer to an existing directory
- Where: When interpreting destination.
- Why: If destination ends with / but that directory does not exist, you must throw and NOT create it.
- How: If destPath.endsWith(path.sep), then verify fs.existsSync(destPath) and fs.statSync(destPath).isDirectory(). If not, throw.
Parent directory existence for file destination
- Where: Before renameSync when destination is treated as a file path (no trailing slash).
- Why: Requirement says do not create directories; parent must exist.
- How: Compute const parent = path.dirname(destPath); if not fs.existsSync(parent) or not a directory, throw.
Proper error signaling
- Where: All error paths and the main entry point.
- Why: Errors must be thrown and process should exit non‑zero; logging alone is insufficient.
- How: Wrap main in try/catch; on catch, console.error(err.message) (optional), then process.exit(1). Avoid swallowing errors.

Minor improvements (non-blocking):

Add small inline comments explaining the directory-vs-file decision logic.

You’ve got a solid starting point with concise use of fs.renameSync and no output on success. Address the above validations and error handling to fully meet the spec. You’re close—once these checks are in place, the app should behave correctly across the required edge cases.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-09-06T17:35:06Z

src/app.js

+const fsCb = require('fs');
+const path = require('path');
+
+const [subject, destination] = process.argv.slice(2);


There is no validation for the number of arguments. The requirements specify that you must verify exactly two arguments are provided (source and destination), and throw an error if not. Please add this validation.

mateacademy-ai-mentor · 2025-09-06T17:35:06Z

src/app.js

+let destinationIsDirectory = false;
+
+try {
+  fileName = path.basename(subject);


There is no check to verify that the source path exists and is a file (not a directory). The requirements specify that you must throw an error if the source does not exist or is not a file. Please add this validation before proceeding.

mateacademy-ai-mentor · 2025-09-06T17:35:06Z

src/app.js

+
+try {
+  fileName = path.basename(subject);
+  destinationIsDirectory = fsCb.statSync(destination).isDirectory();


The code does not handle the case where the destination ends with a slash (/) but does not exist. According to the requirements, if the destination is intended as a directory (ends with /) but does not exist, the app must throw an error and not create directories. Please add this check.

mateacademy-ai-mentor · 2025-09-06T17:35:06Z

src/app.js

+try {
+  fsCb.renameSync(subject, correctDestination);
+} catch (err) {
+  console.error(err);


Errors are only logged to the console, but the requirements specify that errors must be thrown and the process should indicate failure (e.g., exit with a non-zero code). Please update error handling to throw errors and exit with a non-zero code on failure.

mateacademy-ai-mentor · 2025-09-06T17:35:06Z

src/app.js

+  : destination;
+
+try {
+  fsCb.renameSync(subject, correctDestination);


There is no explicit check to ensure that only files are supported (not directories). The requirements specify that the app must not support moving directories. Please add a check to ensure the source is a file and not a directory, and throw an error otherwise.

mateacademy-ai-mentor

Review Summary

Decision: REQUEST_CHANGES

Your CLI logic is solid and aligns well with the requirements for argument parsing, file vs. directory handling, and using fs.renameSync. However, there is one critical blocker: all error paths currently exit with code 0, which violates the requirement that errors must exit with a non-zero status.

Critical issues to fix (blockers):

Incorrect error exit codes in src/app.js:
- At each error path (e.g., invalid argument count, missing source, source is a directory, destination with trailing slash that doesn’t exist), you call process.exit(0). This signals success, which is incorrect for errors.
- Requirement: errors must exit non-zero. Use process.exit(1) instead.
- How to fix: replace each process.exit(0) in error branches with process.exit(1). For example:
  - When argument count is not exactly two (near the top where you validate process.argv), change to process.exit(1).
  - When the source does not exist or is a directory (the fs.statSync/isDirectory() checks), change to process.exit(1).
  - When destination ends with / but directory does not exist, change to process.exit(1).
  - Any other catch-all error paths should likewise use a non-zero exit code.

Minor (optional) improvements:

Make error messages a bit more descriptive (e.g., include the offending path). Not required, but improves UX.

Once you update the exit codes, the implementation should fully meet the requirements. You’re very close—nice work on the overall logic!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button² in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-09-07T07:36:29Z

src/app.js

+if (process.argv.slice(2).length !== 2) {
+  console.error('Usage: node index <source> <destination>');
+
+  process.exit(0);


All error exits in this file use process.exit(0), which signals success. According to the requirements, error conditions (such as invalid arguments, missing files, or invalid destination) must exit with a non-zero code (e.g., process.exit(1)). Please change all process.exit(0) calls in error paths to process.exit(1) to properly signal errors as required.

mateacademy-ai-mentor · 2025-09-07T07:36:29Z

src/app.js

+if (!destinationIsDirectory && destination.endsWith(path.sep)) {
+  console.error('Destination must be correct path');
+
+  process.exit(0);


Same as above: this error exit should use process.exit(1) to indicate an error, not process.exit(0).

mateacademy-ai-mentor · 2025-09-07T07:36:29Z

src/app.js

+
+  if (fsCb.statSync(subject).isDirectory()) {
+    console.error('Subject must be a file');
+    process.exit(0);


Same as above: this error exit should use process.exit(1) to indicate an error, not process.exit(0).

mateacademy-ai-mentor · 2025-09-07T07:36:29Z

src/app.js

+  }
+} catch (subError) {
+  console.error(`subject doesn't exist`);
+  process.exit(0);


Same as above: this error exit should use process.exit(1) to indicate an error, not process.exit(0).

vitalii-bondarenko-ua

Looks good.

mForkosh added 3 commits September 6, 2025 20:27

solution

e64fd2c

fix code

ef65a7b

...

6f0cf4a

mateacademy-ai-mentor suggested changes Sep 6, 2025

View reviewed changes

add fix

6ac6038

mForkosh requested a review from mateacademy-ai-mentor September 7, 2025 07:34

mateacademy-ai-mentor suggested changes Sep 7, 2025

View reviewed changes

vitalii-bondarenko-ua approved these changes Sep 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop#322

Develop#322
mForkosh wants to merge 4 commits intomate-academy:masterfrom
mForkosh:develop

mForkosh commented Sep 6, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Sep 6, 2025

Uh oh!

mateacademy-ai-mentor Sep 6, 2025

Uh oh!

mateacademy-ai-mentor Sep 6, 2025

Uh oh!

mateacademy-ai-mentor Sep 6, 2025

Uh oh!

mateacademy-ai-mentor Sep 6, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Uh oh!

mateacademy-ai-mentor Sep 7, 2025

Uh oh!

mateacademy-ai-mentor Sep 7, 2025

Uh oh!

mateacademy-ai-mentor Sep 7, 2025

Uh oh!

mateacademy-ai-mentor Sep 7, 2025

Uh oh!

vitalii-bondarenko-ua left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mForkosh commented Sep 6, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitalii-bondarenko-ua left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants